✨ [file-based cdk] S3 file format adapter #29353

brianjlai · 2023-08-11T02:02:44Z

What

Adds to the existing S3 adapter to transform an incoming legacy configs into a config that can be used by the v4 connector

How

Given that I don't think any of the other file formats need the adapter I went ahead and added tests and some transformation logic which gets rid of the fields that aren't actually used by the v4 connector.

github-actions · 2023-08-11T02:03:05Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

octavia-squidington-iii · 2023-08-11T06:20:53Z

source-s3 test report (commit `2f827c0334`) - ❌

⏲️ Total pipeline duration: 22mn17s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-11T06:43:20Z

source-s3 test report (commit `2f827c0334`) - ❌

⏲️ Total pipeline duration: 20mn18s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

girarda

Looks good! I forgot about the CsvSpec.advanced_options when we groomed the issue. We should forward them to the file format too

girarda · 2023-08-11T14:12:15Z

airbyte-integrations/connectors/source-s3/source_s3/v4/legacy_config_transformer.py

+        if isinstance(format_options, AvroFormat):
+            return {"filetype": "avro"}
+        elif isinstance(format_options, CsvFormat):
+            csv_options = {


We should also pass the options that are hidden in the CsvSpec.advanced_options

advanced_options["skip_rows"] -> skip_rows_before_header

advanced_options["skip_rows_after_names"] -> skip_rows_after_header

advanced_options["autogenerate_column_names"] -> autogenerate_column_names

got it. will add!

girarda · 2023-08-11T14:25:04Z

airbyte-integrations/connectors/source-s3/source_s3/v4/legacy_config_transformer.py

+                "true_values": ["y", "yes", "t", "true", "on", "1"],
+                "false_values": ["n", "no", "f", "false", "off", "0"],
+                "inference_type": "Primitive Types Only" if format_options.infer_datatypes else "None",
+                "strings_can_be_null": True,


strings_can_be_null should be set from CsvSpec.additional_reader_options["strings_can_be_null"] or default to false https://arrow.apache.org/docs/python/generated/pyarrow.csv.ConvertOptions.html#pyarrow.csv.ConvertOptions

wait so just to confirm, we want it default to false when its not in the additional options?

In the PR description we talked about during planning was: strings_can_be_null -> default to True in legacy

yeah - I must have I derped when we groomed the issue.

default to true in the cdk

default to false in legacy because that's the default in pyarrow

octavia-squidington-iii · 2023-08-11T22:21:02Z

source-s3 test report (commit `868a597e3f`) - ❌

⏲️ Total pipeline duration: 11mn05s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	❌
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	❌

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

girarda

octavia-squidington-iii · 2023-08-14T22:23:23Z

source-s3 test report (commit `e48b9d527b`) - ❌

⏲️ Total pipeline duration: 13mn39s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-14T22:37:36Z

source-s3 test report (commit `bbfbd80cc6`) - ❌

⏲️ Total pipeline duration: 12mn01s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

* [ISSUE airbytehq#28893] infer csv schema * [ISSUE airbytehq#28893] align with pyarrow * Automated Commit - Formatting Changes * [ISSUE airbytehq#28893] legacy inference and infer only when needed * [ISSUE airbytehq#28893] fix scenario tests * [ISSUE airbytehq#28893] using discovered schema as part of read * [ISSUE airbytehq#28893] self-review + cleanup * [ISSUE airbytehq#28893] fix test * [ISSUE airbytehq#28893] code review part #1 * [ISSUE airbytehq#28893] code review part #2 * Fix test * formatcdk * [ISSUE airbytehq#28893] code review * FIX test log level * Re-adding failing tests * [ISSUE airbytehq#28893] improve inferrence to consider multiple types per value * Automated Commit - Formatting Changes * add file adapters for avro, csv, jsonl, and parquet * fix try catch * pr feedback with a few additional default options set * fix things from the rebase of master --------- Co-authored-by: maxi297 <maxime@airbyte.io> Co-authored-by: maxi297 <maxi297@users.noreply.github.com>

maxi297 and others added 20 commits August 4, 2023 13:11

[ISSUE #28893] infer csv schema

eaba483

[ISSUE #28893] align with pyarrow

60757c1

Automated Commit - Formatting Changes

f49235d

[ISSUE #28893] legacy inference and infer only when needed

a394666

[ISSUE #28893] fix scenario tests

4f9d162

[ISSUE #28893] using discovered schema as part of read

0617c82

[ISSUE #28893] self-review + cleanup

d157aa3

[ISSUE #28893] fix test

57b011f

[ISSUE #28893] code review part #1

71cdca9

Merge branch 'master' into issue-28893/infer-schema-csv

ef8f5f5

[ISSUE #28893] code review part #2

f651d03

Fix test

a573a89

formatcdk

0ce37e5

[ISSUE #28893] code review

82db6c3

FIX test log level

3027a4f

Re-adding failing tests

ac91730

[ISSUE #28893] improve inferrence to consider multiple types per value

f1a60ba

Merge branch 'master' into issue-28893/infer-schema-csv

7113603

Automated Commit - Formatting Changes

c9e2004

add file adapters for avro, csv, jsonl, and parquet

e6a0b4d

octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/source/s3 labels Aug 11, 2023

brianjlai changed the base branch from master to issue-28893/infer-schema-csv August 11, 2023 02:03

brianjlai changed the title ~~[file-based cdk] S3 file format adapter~~ ✨ [file-based cdk] S3 file format adapter Aug 11, 2023

fix try catch

2f827c0

brianjlai marked this pull request as ready for review August 11, 2023 05:56

octavia-squidington-iii removed the CDK Connector Development Kit label Aug 11, 2023

brianjlai requested a review from a team August 11, 2023 05:56

brianjlai requested review from maxi297 and clnoll August 11, 2023 05:57

girarda reviewed Aug 11, 2023

View reviewed changes

pr feedback with a few additional default options set

868a597

girarda approved these changes Aug 14, 2023

View reviewed changes

Base automatically changed from issue-28893/infer-schema-csv to master August 14, 2023 19:14

Merge branch 'master' into brian/s3_csv_format_adapter

e48b9d5

octavia-squidington-iii added the CDK Connector Development Kit label Aug 14, 2023

fix things from the rebase of master

bbfbd80

octavia-squidington-iii removed the CDK Connector Development Kit label Aug 14, 2023

brianjlai merged commit 82b8274 into master Aug 14, 2023

brianjlai deleted the brian/s3_csv_format_adapter branch August 14, 2023 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [file-based cdk] S3 file format adapter #29353

✨ [file-based cdk] S3 file format adapter #29353

brianjlai commented Aug 11, 2023

github-actions bot commented Aug 11, 2023 •

edited by brianjlai

Loading

octavia-squidington-iii commented Aug 11, 2023

octavia-squidington-iii commented Aug 11, 2023

girarda left a comment

girarda Aug 11, 2023

brianjlai Aug 11, 2023

girarda Aug 11, 2023

brianjlai Aug 11, 2023

girarda Aug 11, 2023

octavia-squidington-iii commented Aug 11, 2023

girarda left a comment

octavia-squidington-iii commented Aug 14, 2023

octavia-squidington-iii commented Aug 14, 2023

✨ [file-based cdk] S3 file format adapter #29353

✨ [file-based cdk] S3 file format adapter #29353

Conversation

brianjlai commented Aug 11, 2023

What

How

github-actions bot commented Aug 11, 2023 • edited by brianjlai Loading

Before Merging a Connector Pull Request

octavia-squidington-iii commented Aug 11, 2023

source-s3 test report (commit 2f827c0334) - ❌

octavia-squidington-iii commented Aug 11, 2023

source-s3 test report (commit 2f827c0334) - ❌

girarda left a comment

Choose a reason for hiding this comment

girarda Aug 11, 2023

Choose a reason for hiding this comment

brianjlai Aug 11, 2023

Choose a reason for hiding this comment

girarda Aug 11, 2023

Choose a reason for hiding this comment

brianjlai Aug 11, 2023

Choose a reason for hiding this comment

girarda Aug 11, 2023

Choose a reason for hiding this comment

octavia-squidington-iii commented Aug 11, 2023

source-s3 test report (commit 868a597e3f) - ❌

girarda left a comment

Choose a reason for hiding this comment

octavia-squidington-iii commented Aug 14, 2023

source-s3 test report (commit e48b9d527b) - ❌

octavia-squidington-iii commented Aug 14, 2023

source-s3 test report (commit bbfbd80cc6) - ❌

github-actions bot commented Aug 11, 2023 •

edited by brianjlai

Loading

source-s3 test report (commit `2f827c0334`) - ❌

source-s3 test report (commit `2f827c0334`) - ❌

source-s3 test report (commit `868a597e3f`) - ❌

source-s3 test report (commit `e48b9d527b`) - ❌

source-s3 test report (commit `bbfbd80cc6`) - ❌